Skip to content

Fix duplicating each entry if a list of bools is returned#48

Open
mgotz wants to merge 2 commits intoklemenv:mainfrom
hz-b:bool_array_fix
Open

Fix duplicating each entry if a list of bools is returned#48
mgotz wants to merge 2 commits intoklemenv:mainfrom
hz-b:bool_array_fix

Conversation

@mgotz
Copy link
Contributor

@mgotz mgotz commented Mar 3, 2026

I noticed a bug, when returning a list of bools from python. Each entry would apear doubly in the EPICS waveform.

Take this database:

record(waveform, "bools") {
    field(DTYP, "pydev")
    field(FTVL, "CHAR")
    field(NELM, 10)
    field(PINI, "YES")
    field(INP, "@[True, False]*5")
}

The waveform should contain: 1,0,1,0,1,0,1,0,1,0. Instead it contains: 1,1,0,0,1,1,0,0,1,1

Python bools are implemented as integers.
Thus in pywrapper.cpp conversion the PyLong_Check matches and the PyBool_Check as well and each entry is pushed to val twice.
Since Bools are integers under the hood anyhow, I think there is no need for an extra case.

PyBool is implemented as a subclass of int, thus bools were added twice to the val.
Once from PyLong_Check and once from PyBool_Check.
Treating bools separately is unnecessary because they are treated fine as long.
@klemenv
Copy link
Owner

klemenv commented Mar 3, 2026

Good catch. There's actually a possibility for a similar issues between long vs int in Python2.

Could you change your patch to instead move the PyBool_Check before the PyIn_Check and then add ''continue" into every " if" block in the vector for loop.

@mgotz
Copy link
Contributor Author

mgotz commented Mar 4, 2026

I changed the type checks inside the vector loop to else if statements. To me this is the much clearer construct than using if with continue.
If you want I'll readd the PyBool_Check, but it is redundant. In Python2 Bools are also implemented as integers and all we need here is to have an integer value for the bool.

@klemenv
Copy link
Owner

klemenv commented Mar 8, 2026

Let's stick with 'continue'. I prefer 'continue', 'break', 'return' in long code blocks like this one because when reading the code, upon encountering 'continue' one can be ensured that even if there was any other code after if...else if...else, it would be skipped. In a sense it keeps readers' focus on the code block at hand only, instead of scrolling up and down a lot.

@mgotz
Copy link
Contributor Author

mgotz commented Mar 10, 2026

Fair enough. I changed it to continue.

I noticed another inconsistency in the type conversion. Not sure if this should be tacked on here, made separate or simply ignored.
For the PyInt_Check there is:

long long val = PyInt_AsLong(el);

where as for the PyLong_Check it is:

long val = PyLong_AsLong(el);

For the PyLong it could be

long long val = PyLong_AsLongLong(el);

And simply long type for the PyInt. That would be consistent with the conversions done for the scalars.
But probably doesn't make a difference on most modern systems where long and long long are both 64 bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants